Repository navigation
fix(app): stop expanded rows corrupting virtualized row heights - #3171
Conversation
A row and its inline expansion are two `tr`s sharing one virtual index, and both carried `data-index` and the virtualizer's `measureElement` ref. Since virtual-core keys its element cache by index, the expanded `tr` took over that index's ResizeObserver registration and cached its own height there. Collapsing called the ref with `null`, which `measureElement` ignores, so the inflated height stuck and nothing observed the row any more. Each expand/collapse then shrank the render window until scrolling showed a few rows above a screen of empty space. Wrap the pair in a `tbody` and measure that instead, so one element maps to one virtual index. Co-authored-by: Mike Shi <mike@hyperdx.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 970bd3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
 |
E2E Test Results✅ All tests passed • 381 passed • 1 skipped • 1124s
Tests ran across 4 shards in parallel. |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Deep Review✅ No critical issues found. This is a well-contained, correctly-reasoned virtualization fix. Moving 🟡 P2 — recommendedNone. 🔵 P3 nitpicks (2)
Reviewers: orchestrator direct diff analysis, performance, testing (both returned clean). Also dispatched but not yet returned at synthesis time: correctness, adversarial, frontend-races, kieran-typescript, maintainability, project-standards, agent-native, learnings-researcher, previous-comments. Prior PR comments were all automated (Vercel, changeset-bot, Greptile 5/5, passing E2E) with no actionable human feedback to verify. Testing gaps: Regression protection is entirely behavioral (e2e); no component-level test pins the one- No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .poll(async () => { | ||
| const m = await searchPage.table.getVirtualizationMetrics(); | ||
| // Rows still below, so the rendered window must cover the viewport. | ||
| return m.remainingBelowPx > 0 ? m.visibleGapPx : 0; |
There was a problem hiding this comment.
🔵 minor — remainingBelowPx > 0 guard lets the gap assertion pass vacuously
remainingBelowPx is scrollHeight - scrollTop - clientHeight, which counts the footer <tbody> (the "End of Results"/loading banner, DBRowTable.tsx:1293-1390) and the virtualizer's paddingEnd: 20 as content below — it does not mean "rows still below". If the seeded search returns few enough rows that the table doesn't overflow, scrollTo(1200) clamps to 0, remainingBelowPx is 0, and all three offsets return 0 and pass without measuring anything; conversely an offset landing inside the footer band makes visibleGapPx positive on a correctly rendered table. Assert a precondition up front (e.g. remainingBelowPx > 0 and renderedRows > 20 before the loop, as test 1 does) and derive the offsets from the measured scroll range rather than hardcoding 300/700/1200.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // expand/collapse shrank the render window until the viewport went blank. | ||
| test('collapsing an expanded row restores the table height', async () => { | ||
| const before = await searchPage.table.getVirtualizationMetrics(); | ||
| expect(before.renderedRows).toBeGreaterThan(20); |
There was a problem hiding this comment.
🔵 minor — Exact-equality baseline captured before the virtualizer settles
before is read immediately after the first row becomes visible, then used as an exact expected value at line 35. Row heights are still settling at that point — the table renders in var(--font-ibm-plex-mono) (packages/app/styles/LogTable.module.scss:24), so a font swap after the baseline reading re-measures every row and shifts the steady-state window size by a row or two, and the poll at line 30 then times out on a healthy table. Poll the baseline until two consecutive readings agree (or assert visibleGapPx <= 0 / renderedRows within a tolerance) instead of pinning an exact count taken from the first paint.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // the element registered for this row's virtual index. | ||
| <tr | ||
| data-testid={`expanded-row-${rowId}`} | ||
| key={`${virtualKey}-expanded`} |
There was a problem hiding this comment.
🔵 minor — virtualKey prop is now dead
With measureElement/virtualIndex gone, virtualKey's only remaining use is key={${virtualKey}-expanded} on the component's single root element, which has no reconciliation effect — the key that matters is on the <tbody> at DBRowTable.tsx:1153. Drop the prop and the virtualKey={virtualRow.key.toString()} argument at DBRowTable.tsx:1278.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 3 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
…3117) Resolves HDX-5365. > **Stacked on #3171.** Base is `cursor/fix-virtualized-row-height-d7eb`; it will retarget to `main` once that merges. See "Virtualized row heights" below. ## Why The only way to expand a search result row inline is the 16px chevron, which is a poor hit target when you are scanning logs during an incident. Comparable tools expand on row click. Rather than change what a row click does for everyone, this adds a preference. **The default is unchanged** — a row click still opens the side panel. Users who want the bigger hit target can opt in. ## What changed **Preferences → Search results → Row click**, defaulting to `Open side panel`. Set it to `Expand inline` and: - a row click toggles inline expansion instead of opening the side panel; - the side panel moves to a hover button on the row (`Open in side panel`, beside the existing copy buttons), and the expanded row's maximize button still opens it; - row clicks keep driving the side panel while it is open, so browsing row to row inside the panel is unchanged. Closing the panel returns clicks to inline expansion. Stored as an optional `rowClickAction`. Stored preferences that predate the field are returned as-is rather than merged with the defaults, so readers fall back to `DEFAULT_ROW_CLICK_ACTION`. Two table variants need care regardless of the preference: - Tables that hide the expand column (surrounding context, patterns) have nowhere to put inline details, so they always open the side panel. - The ClickHouse dashboard's slowest-queries list renders inline details but passes no `onRowDetailsClick`, so it always expands inline and offers neither the hover button nor the expanded row's maximize button — both would only write URL params nothing reads. Routing a row click needs to know whether a side panel is available and whether one is already open, and two signals did not actually mean that: - `highlightedLineId` was passed to the table even when the panel was not mounted. `rowWhere`/`rowSource` survive a source switch while closing the panel clears both, so a stale `rowWhere` left the table highlighting a row with no panel on screen. The panel is now hoisted into one `sidePanel` element that both its own mount and `highlightedLineId` derive from. - `ExpandedLogRow` gated its maximize button on `{openSidebar && …}`, the return value of a `useCallback` and so always truthy. It now takes `canOpenSidePanel`. `DBRowTableIconButton` also gained an `aria-label` from its title, which the row hover buttons had been missing. ## Virtualized row heights Reported against this branch: expand and collapse a few rows, scroll, and the table goes blank under the header. That bug is on `main` today — it reproduces identically through the chevron, with this PR's `packages/app/src` reverted. A row and its inline expansion are two `tr`s sharing one virtual index, and both were measured, so the expanded one took over that index's `ResizeObserver` registration and left its height cached there after collapsing. Every expand/collapse shrank the render window. Fixed separately in **#3171** so it lands for chevron users regardless of this PR, and merged in here so this branch does not ship the bigger hit target on top of it. That merge is the only reason `DBRowTable.tsx` now renders a `tbody` per row. ## Tests - Seven unit tests in `DBRowTable.test.tsx`: the side panel on row click by default (and no redundant hover button in that mode), then under `expand` — inline expand/collapse on body click, the hover button, `showExpandButton={false}`, and the open-panel case; plus the no-side-panel table expanding inline regardless of the preference. These are the first tests in that file to render virtualized body rows, hence the geometry mocks. - The inline-expand test also asserts the expanded row exposes `Open in side panel` and that clicking it writes the `rowWhere` param, so `canOpenSidePanel` is pinned in both directions — hardcoding it to `false` fails the test rather than staying green. - `UserPreferencesModal.test.tsx` drives the select: the default for users with no stored preference, a stored value round-tripping into the control, and picking an option writing `rowClickAction`. A wrong storage key or option value now fails instead of passing green. - E2E: the specs that reach the side panel go back to a plain row click, since that is the default again. A new `Row click set to expand inline` describe seeds the preference before navigating and asserts a row body click expands inline without opening the panel, collapses on a second click, and that the hover button opens the panel. - `virtualized-row-heights.spec.ts` comes in from #3171 and covers the virtualizer regression. ## Rebase onto `main` Rebased past #3140 and #3164, which added row multi-select and hover-revealed selection checkboxes to the same row component. No textual conflicts, and the two features compose: the checkbox lives in its own cell and stops propagation, so it does not compete with the row-body click. CI lint was failing only because this branch predated the `--max-warnings` bump on `main`. The rebase picks that up; the one warning this PR did add (an unsafe cast in the new `Select` handler) is gone, so the app count is back under the ceiling rather than exactly on it. Verification: `make ci-lint` and `make ci-unit` green (244 suites, 3948 tests after the merge). All 94 `search/` E2E tests re-run — 85 passed, 1 flaky-then-passed, and 2 `search-filters.spec.ts` failures that reproduce identically with this PR's `packages/app/src` reverted to `origin/main`, so they are environmental and not from this change. `correlated-metric-source`, `span-links` and `traces-workflow` also pass. ## Walkthrough The reported repro, end to end on this branch: two row clicks opening the side panel on the default setting, switching the preference to `Expand inline`, expanding and collapsing six rows, then scrolling — the table stays full. <a href="https://cursor.com/agents/bc-b2c91bdd-e466-447c-942d-dce15159d7eb/artifacts?path=%2Fopt%2Fcursor%2Fartifacts%2Fvirtualized_scroll_repro_fixed.mp4"><img src="https://cursor.com/artifacts/c/art-27481ff4-27fa-4bca-bbe1-3c2abee4f889" alt="virtualized_scroll_repro_fixed.mp4" /></a> Default — a row click opens the side panel, unchanged:  The opt-in:  With it on, a row click expands in place, with no side panel:  Hovering a row shows both features side by side — the selection checkbox on the left, and this PR's `Open in side panel` button among the row action icons on the right:  Linear Issue: [HDX-5365](https://linear.app/clickhouse/issue/HDX-5365/expand-log-inline-on-row-click) <div><a href="https://cursor.com/agents/bc-b2c91bdd-e466-447c-942d-dce15159d7eb?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-b2c91bdd-e466-447c-942d-dce15159d7eb&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> Co-authored-by: Cursor Agent <199161495+cursoragent@users.noreply.github.com>
![https://cursor.com/assets/images/open-in-web-dark.png"><source](https://cursor.com/assets/images/open-in-web-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-web-light.png"><img](https://cursor.com/assets/images/open-in-web-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a](https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"><source](https://cursor.com/assets/images/open-in-cursor-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-cursor-light.png"><img](https://cursor.com/assets/images/open-in-cursor-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div](https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div){kind=link}
Summary
Expanding and then collapsing rows in the search results table permanently shrinks the virtualizer's render window. After a few expand/collapse cycles, scrolling leaves a screen of empty space with only a handful of rows in it — sometimes none at all, while the result count still reads 127.
Root cause. A row and its inline expansion are two
<tr>s but one virtual item. Both carrieddata-index={virtualRow.index}and the virtualizer'smeasureElementref:virtual-corekeysmeasureElementCachebyitem.key, which is the index. When the expanded<tr>mounts,_measureElementseesprevNode !== node, unobserves the base row, observes the expanded row, and caches the expanded row's height as the size of that index.null, andmeasureElementreturns early fornull. TheResizeObserverdoes fire for the detached node, but that path takes the!node.isConnectedbranch, which unobserves and returns before resizing.So the index keeps the expanded height forever, and nothing observes that row any more, so it never re-measures. Every expand/collapse inflates one more index.
Fix. Wrap each row and its expansion in their own
<tbody>and measure that instead, so one DOM element maps to one virtual index. TheResizeObserveron the<tbody>picks up both the row and the expansion growing or going away, so collapsing restores the correct height on its own. No stylesheet targets this table'stbody, and multipletbodyelements are valid in atable.This is not a new regression — it reproduces on
maintoday via the chevron. The expand/collapse affordance just makes it easy to hit.Screenshots or video
Six expand/collapse cycles on rows 0–5, then scrolling to the same offset. Left is
main, right is this branch:Measured on the scroll container during that run:
scrollTop1200scrollTop1200Negative means the rendered rows overfill the viewport, which is what you want. The viewport is 555px tall, so a +941px gap means nothing at all is visible.
Tests
packages/app/tests/e2e/features/search/virtualized-row-heights.spec.tscovers both halves: that collapsing restores the render window, and that scrolling afterwards keeps the viewport covered. Both fail onmain(Expected: 55, Received: 38;Expected: <= 0, Received: 86) and pass here.make ci-lintandmake ci-unitare green. The fullsearch/E2E directory was re-run: 84 passed, 3 flaky-then-passed, and 2search-filters.spec.tsfailures that reproduce identically withpackages/app/srcreverted toorigin/main, so they are environmental rather than from this change.How to test on Vercel preview
Preview routes: /search
Steps:
References
Linear Issue: HDX-5365